Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor messages/agency_comm module, delete legacy proof #149

Merged
merged 7 commits into from
Nov 4, 2020

Conversation

Patrik-Stas
Copy link
Contributor

@Patrik-Stas Patrik-Stas commented Oct 18, 2020

Bunch of cleanusp and refactors packed here:

  • Deleted legacy proof code (code around ProofRequestMessage)

  • Renamed messages to agency_comm to clearly signal the module is dealing with communication with agency

  • Took src/disclosed_proof_utils.rs and src/proof_utils and along some still-used-datastructures which was located along with legacy proofs src/messages/proofs/*.rs restructured them into newly created module libindy (see src/libindy/**).

  • Also moved utils/libindy to libindy/utils. I think all libindy wrapping code should be in one place to be able to modularize credentials/ledgers and possibly swap implementations in the future. So this would be just a beginning, there still fragments of libindy wrapping code in utils/ and other part of codebase

  • Started breaking up global settings - I've created src/agency_comm/agency_settings.rs which looks a lot like the src/settings.rs, but only deals with CONFIG_AGENCY_ENDPOINT, CONFIG_AGENCY_DID, CONFIG_AGENCY_VERKEY, CONFIG_REMOTE_TO_SDK_DID, CONFIG_REMOTE_TO_SDK_VERKEY, CONFIG_SDK_TO_REMOTE_DID, CONFIG_SDK_TO_REMOTE_VERKEY.

    Removing dependency on ::settings us closer toward making agency_comm standalone module which could be used without using the rest of aries-vcx.

    Naturally I've removed these from the src/settings.rs. To keep things working from user perspective, some calls into src/settings.rsare propagated further to src/agency_comm/agency_settings.rs such as fn validate_config, fn set_testing_defaults, fn clear_config, fn process_config_string.

@Patrik-Stas Patrik-Stas requested a review from a team as a code owner October 18, 2020 11:45
@codecov-io
Copy link

codecov-io commented Oct 18, 2020

Codecov Report

Merging #149 into master will decrease coverage by 0.06%.
The diff coverage is 58.20%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #149      +/-   ##
==========================================
- Coverage   50.86%   50.79%   -0.07%     
==========================================
  Files         151      156       +5     
  Lines       23247    22918     -329     
  Branches     6138     6091      -47     
==========================================
- Hits        11825    11642     -183     
+ Misses       7754     7647     -107     
+ Partials     3668     3629      -39     
Flag Coverage Δ
unittests 50.79% <58.20%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
libvcx/src/agency_comm/payload.rs 0.00% <ø> (ø)
libvcx/src/agency_comm/thread.rs 71.87% <ø> (ø)
libvcx/src/agency_comm/update_message.rs 27.77% <0.00%> (ø)
libvcx/src/api/credential.rs 33.56% <ø> (ø)
libvcx/src/api/credential_def.rs 51.12% <ø> (-0.20%) ⬇️
libvcx/src/api/disclosed_proof.rs 41.04% <0.00%> (-1.34%) ⬇️
libvcx/src/api/issuer_credential.rs 36.88% <ø> (ø)
libvcx/src/api/return_types_u32.rs 64.06% <ø> (ø)
libvcx/src/api/schema.rs 56.32% <ø> (-0.25%) ⬇️
libvcx/src/api/wallet.rs 49.76% <ø> (ø)
... and 87 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 49e7732...cef5209. Read the comment docs.

Copy link
Contributor

@mirgee mirgee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think agency_vcx is way more confusing and inaccurate than messages. How about something like comm? Or, if you want to mention agency, than something like agency_comm or agency_messages... This is just throwing ideas out there.

@Patrik-Stas Patrik-Stas changed the title Cleanup/messages WIP: Cleanup/messages Oct 26, 2020
@Patrik-Stas Patrik-Stas force-pushed the cleanup/messages branch 3 times, most recently from 809d2e5 to fcfbfc8 Compare October 30, 2020 16:26
@Patrik-Stas Patrik-Stas changed the title WIP: Cleanup/messages Cleanup/messages Oct 30, 2020
@Patrik-Stas Patrik-Stas changed the title Cleanup/messages Cleanup/messages, delete legacy proof Nov 1, 2020
@Patrik-Stas Patrik-Stas changed the title Cleanup/messages, delete legacy proof wip: Cleanup/messages, delete legacy proof Nov 1, 2020
@Patrik-Stas Patrik-Stas changed the title wip: Cleanup/messages, delete legacy proof Refactor messages/agency_comm module Nov 1, 2020
@Patrik-Stas Patrik-Stas changed the title Refactor messages/agency_comm module Refactor messages/agency_comm module, delete legacy proof Nov 1, 2020
libvcx/src/libindy/proofs/proof_request.rs Outdated Show resolved Hide resolved

pub static DEFAULT_DID: &str = "2hoqvcwupRTUNkXn6ArYzs";
pub static DEFAULT_VERKEY: &str = "FuN98eH2eZybECWkofW6A9BKJxxnTatBCopfUiNxo6ZB";
pub static DEFAULT_URL: &str = "http://127.0.0.1:8080";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be preferrable to have all config-related constants on one place (although splitting settings.rs is not a bad idea), unless they were used only in this module

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I disagree about placement to settings.rs as the idea is to make agency_comm standalone, thanks to your comment I've noticed that these things actually do not need to be static at all. These are only used within set_testing_defaults_agency function in agency_settings.rs, so I am moving it right there.

Copy link
Contributor

@mirgee mirgee Nov 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't say anything about putting to settings.rs, by one place I mean e.g. two submodules of a single module. I see your point about benefits of making agency-related logic standalone, but these configs are going to be used throughout libvcx even in case agency_comm were to become separate crate, they are part of the interface between libvcx and agency_comm, and thus are global to the app in that sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, these particular here are default values used in agency tests, and as I've found, really just local to 1 single test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By "config-related constants" I meant the config keys as well. But if the plan is to get rid of agency_settings.rs anyways (which I agree with), let's keep it as it is for now.

libvcx/src/agency_comm/agency_settings.rs Outdated Show resolved Hide resolved
libvcx/src/agency_comm/agency_settings.rs Outdated Show resolved Hide resolved
libvcx/src/agency_comm/agency_settings.rs Show resolved Hide resolved
//Todo: Update p_type to use Enum
pub p_type: String,
pub p_value: i32,
#[serde(skip_serializing_if = "Option::is_none")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why is skip_serializing_if necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I haven't done modifications here, just moved the datastructure, however I suppose it makes sense to skip serializing the field if there's no value set.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why is it necessary? Extra fields are ignored during deserialization...

Copy link
Contributor Author

@Patrik-Stas Patrik-Stas Nov 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not ignored

use serde_json;

#[derive(Serialize)]
struct Data {
    #[serde(skip_serializing_if = "Option::is_none")]
    b: Option<String>,
    c: Option<String>,
}

pub fn run()
{
    let data = Data { b: None, c:None};
    let serialized = serde_json::to_string(&data).unwrap();
    println!("serialized={}", serialized);
}
serialized={"c":null}

Copy link
Contributor

@mirgee mirgee Nov 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I said "deserialization", not "serialization" :) I was just wondering if there is any actual functional reason why these specific field must be skipped if none, but I guess this is just a sporadic attempt to reduce the payload size...

libvcx/src/agency_comm/create_key.rs Outdated Show resolved Hide resolved
libvcx/src/libindy/proofs/mod.rs Show resolved Hide resolved
libvcx/src/settings.rs Outdated Show resolved Hide resolved
config.clear();
agency_settings::clear_config_agency()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the reasons all settings should share a module, IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disagree, this "redirection" is to simply preserve aries-vcx functionality as a whole, but removing dependencies on aries-vcx globals is precondition to create independent modules.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by redirection? I think that on the contrary, libvcx, as a "user" of agency_comm, has the responsibility of configuring it as necessary. As I see it, agency_settingsis a subset of libvcx settings, not separate from it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By redirection I meant that call into the src/settings.rs further calls agency_settings.rs.

But yeah, it could be removed and code in all places would have to make calls into agency_settings.rs manually when needed. It was more convenient to implement this way, with "the redirection", but I also don't see this as ultimate state. I think we could eliminate agency_settings.rs completely in the next steps.

- Move messages::proofs to indyvc::proofs, delete legacy code
- Rename module messages to agency_vcx
- Start removing depending on ::settings from ::agency_vcx
- Remove concept of protocol_type from ::agency_vcx
- Ignore legacy tests, fix compile errors

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
Signed-off-by: Patrik Stas <patrik.stas@absa.africa>
@mirgee mirgee merged commit e0c4d93 into master Nov 4, 2020
@mirgee mirgee deleted the cleanup/messages branch November 4, 2020 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants